fix(security): report the CSP floor before enforcing it - #3455
Conversation
#3417 shipped the platform CSP floor enforced. It blocked, on every hosted project that loads an asset the platform does not itself emit, every one of those assets at once. A fleet audit finds ~100 projects in that state -- stock photography (images.unsplash.com, i.pravatar.cc, picsum.photos), video (player.vimeo.com, vjs.zencdn.net), object storage (storage.googleapis.com), embeds (www.youtube.com) -- including veryfront's own marketing site. Only two projects, codersociety and tomcode, declare `security.csp` at all. The owner gets no signal beyond a browser console, and no remedy short of a republish: `security.csp` lives in project config, and a hosted project serves config from its deployed release, so editing it changes nothing until the next publish. codersociety hit exactly this -- added the three directives it needed at 04:57, still served the bare floor hours later because its production release was cut in June. So the floor reports before it enforces. A project that has declared `security.csp` has demonstrably looked at its policy and keeps the enforced header; everyone else gets `Content-Security-Policy-Report-Only`, which surfaces identical violations without breaking the page. The reported policy is byte-identical to the enforced one, so what a project sees now is exactly what it will get later -- there is a test pinning that. Two escape hatches stay enforced: `VERYFRONT_CSP_ENFORCE` flips the default once adoption is high enough, and a full `VERYFRONT_CSP` override is enforced because writing one by hand is already an explicit ops act. An empty `csp: {}` counts as opt-in. The signal is that the project touched the key, not how much it put in it; reading emptiness as "unconfigured" would strand a project that deliberately accepted the floor in report-only forever. This is a narrow, time-boxed relaxation, not a retreat: the floor was enforced for roughly one day, so report-only restores the state the fleet was already in rather than weakening a settled protection. `content-security-policy-report-only` joins the policy-owned header list, so a project-provided value cannot survive into a response whose policy the platform is deciding. Verified: full unit suite green -- 3811 passed, 27995 steps, 0 failed. Three handler tests read the policy header directly and now accept either name; they assert nonce alignment, not enforcement mode. deno check, lint and fmt clean.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe security handler now emits generated CSP as report-only by default and as enforced CSP when configuration or environment overrides require enforcement. Tests accept either CSP header and cover both delivery modes. ChangesCSP delivery modes
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant CSPHeaderSelection
participant ApplySecurityHeaders
participant Response
Request->>CSPHeaderSelection: Provide configuration and environment state
CSPHeaderSelection-->>ApplySecurityHeaders: Select enforced or report-only header
ApplySecurityHeaders->>Response: Emit generated CSP policy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/http/response/security-handler.ts`:
- Around line 26-29: Update the extraHeaders loop in applySecurityHeaders to
skip both case-insensitive Content-Security-Policy and
Content-Security-Policy-Report-Only names, preventing project values from
replacing generated CSP headers or their delivery mode. Add regression tests
covering a custom value for each header name and verify the generated platform
policy remains effective.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f33d7b77-41bb-4c85-aff0-4be22176bb68
📒 Files selected for processing (5)
src/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tssrc/server/handlers/request/openapi-docs.handler.test.tssrc/server/handlers/request/rsc/index.test.tssrc/server/handlers/request/static.handler.test.ts
Three follow-ups to the same behaviour change, all found by CI rather than by the unit suite -- integration tests are not part of `test:unit`, which is what was run locally. Four assertions in the production-server integration tests read `content-security-policy` directly. Their fixtures declare no `security.csp`, so the floor now reaches them report-only. They assert policy content and nonce alignment, not enforcement mode, so they read whichever header carries the policy via a local `readCsp` helper. The mode itself is covered by dedicated cases in the unit tests. `cspHeaderName` is no longer exported. Nothing outside this module calls it -- it was exported only out of habit, and exporting it widened the public API surface and the generated reference for no gain. Regenerate `docs/api-reference/veryfront/security.md`: adding the enforcement helper shifted the line pins the reference cites. Regenerated with the same Deno the CI check pins (2.7.7), never hand-edited.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/server/production-server.test.ts`:
- Around line 34-37: Update the helper comment above the served-policy logic to
document that VERYFRONT_CSP and VERYFRONT_CSP_ENFORCE may select enforced
delivery even when fixtures omit security.csp, while preserving the note that
assertions validate policy content and nonce alignment rather than delivery
mode.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d7f9906-93f1-4423-9556-3b73b5c15100
📒 Files selected for processing (3)
docs/api-reference/veryfront/security.mdsrc/security/http/response/security-handler.tstests/integration/server/production-server.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/security/http/response/security-handler.ts
Review catch on the report-only change, and a pre-existing hole it would have widened. `applySecurityHeaders` ends by writing every `security.headers` entry straight onto the response. Every other name that loop can touch has a deliberate override path through `getHeaderOverride` -- Referrer-Policy, X-Frame-Options, and so on -- so re-setting them is the intended behaviour. CSP has no such path: it is computed from the platform floor merged with `security.csp`, and a value arriving through `security.headers` silently replaced the whole policy. That was already true before this branch; adding a second delivery name meant a project could also flip whether the policy is enforced or merely reported. Skip both CSP names, case-insensitively, and warn once when one is dropped so the project learns where the policy actually comes from. `isSecurityPolicyResponseHeaderName` is deliberately not reused for the check. It covers the whole policy-owned set, including the headers this loop is still meant to override; matching on it would silently remove working overrides. Worth noting that helper currently has no call sites at all, so the "server integrations must remove project-provided values" contract its doc comment describes is not enforced anywhere -- out of scope here, but it is why this had to be handled at the point of use. Three tests cover it: a project CSP value is ignored while its `security.csp` still merges, the report-only name is ignored case-insensitively, and the legitimate overrides plus arbitrary custom headers still land. Also correct the integration helper comment: `VERYFRONT_CSP` and `VERYFRONT_CSP_ENFORCE` select enforced delivery too, not just a `security.csp` declaration. Verified: 80 steps in the security-handler suite, 0 failed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/security/http/response/security-handler.test.ts`:
- Around line 809-820: Update the test case in the “ignores a project-supplied
report-only header, matching case-insensitively” block to use a mixed-case
Content-Security-Policy-Report-Only key instead of the all-lowercase form, while
preserving the existing assertions.
In `@src/security/http/response/security-handler.ts`:
- Around line 425-429: Update applySecurityHeaders so the ignoredCspHeader
warning is emitted only once, reusing the existing reserved-CORS warning
suppression pattern and state rather than logging on every response. Preserve
the current warning message and CSP detection behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c337cfa1-34b3-416c-8213-8395926fe56c
📒 Files selected for processing (3)
src/security/http/response/security-handler.test.tssrc/security/http/response/security-handler.tstests/integration/server/production-server.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/server/production-server.test.ts
Adding the enforcement helper and the CSP header guard shifted the line pins the generated reference cites. Regenerated with the Deno the CI check pins (2.7.7); never hand-edited.
…e-insensitively Two review catches, both valid. `applySecurityHeaders` runs once per response, so the warning for an ignored `security.headers` CSP entry fired on every request a misconfigured project served. Guarded with the same WeakSet suppression the reserved-CORS warning next to it already uses, keyed on the headers object so one config warns once. The report-only test used a lowercase key, which is already the form the lookup normalizes to -- a regression that dropped case-normalization would have passed it. Switched to `Content-Security-Policy-Report-Only` so the test exercises the contract it claims to. Docs regenerated: the suppression changed line pins in the generated API reference again. 80 steps, 0 failed.
What
Serve the platform CSP floor as
Content-Security-Policy-Report-Onlyuntil a project opts in by declaringsecurity.csp. Projects that have declared it keep the enforced header.Why
#3417 shipped the floor enforced. A fleet audit finds ~100 hosted projects whose assets it blocks — stock photography (
images.unsplash.com,i.pravatar.cc,picsum.photos), video (player.vimeo.com,vjs.zencdn.net), object storage (storage.googleapis.com), embeds (www.youtube.com) — including veryfront's own marketing site. Only two projects,codersocietyandtomcode, declaresecurity.cspat all.The owner gets no signal beyond a browser console, and no remedy short of a republish:
security.csplives in project config, and a hosted project serves config from its deployed release, so editing it changes nothing until the next publish. codersociety hit exactly this — added the three directives it needed at 04:57, still served the bare floor hours later because its production release was cut in June. Its entire video library was dead in production.Behaviour
security.csp(incl.csp: {})Content-Security-PolicyContent-Security-Policy-Report-OnlyVERYFRONT_CSP_ENFORCEsetContent-Security-Policy(all projects)VERYFRONT_CSPoverride setContent-Security-PolicyThe reported policy is byte-identical to the enforced one — there is a test pinning that, so what a project sees now is exactly what it gets when the flag flips.
csp: {}counts as opt-in: the signal is that the project touched the key, not how much it put in it. Reading emptiness as "unconfigured" would strand a project that deliberately accepted the floor in report-only forever.Trade-off
This does relax a security control, and reviewers should weigh that. The floor was enforced for roughly one day, so report-only restores the state the fleet was already in rather than weakening a settled protection — and it converts a silent fleet-wide breakage into visible violation reports.
VERYFRONT_CSP_ENFORCEends the rollout whenever adoption justifies it.Notes
content-security-policy-report-onlyjoinsSECURITY_POLICY_RESPONSE_HEADER_NAMES, so a project-provided value cannot survive into a response whose policy the platform is deciding.Three handler tests read the policy header directly and now accept either name — they assert nonce alignment, not enforcement mode.
Verification
Full unit suite green: 3811 passed, 27995 steps, 0 failed.
deno check,lint,fmtclean.Summary by CodeRabbit
New Features
Tests
Documentation